checker(dm): check the number of connections before starting #5185

buchuitoudegou · 2022-04-15T07:50:17Z

What problem does this PR solve?

Issue Number: close #5005

What is changed and how it works?

add new checker to check the number of connections

Check List

Tests

Unit test
Integration test

Code changes

Side effects

Related changes

Release note

`None`

ti-chi-bot · 2022-04-15T07:50:18Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

buchuitoudegou · 2022-04-15T07:50:57Z

Will add ut later

codecov-commenter · 2022-04-15T08:18:20Z

Codecov Report

Merging #5185 (c48b5cb) into master (6290709) will increase coverage by 0.0419%.
The diff coverage is 57.7092%.

Flag	Coverage Δ
cdc	`66.0872% <65.6862%> (-0.0258%)`	⬇️
dm	`52.1090% <51.2000%> (+0.0727%)`	⬆️
engine	`63.8494% <ø> (+0.1077%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #5185        +/-   ##
================================================
+ Coverage   59.4214%   59.4633%   +0.0419%     
================================================
  Files           763        767         +4     
  Lines         87418      87760       +342     
================================================
+ Hits          51945      52185       +240     
- Misses        30894      30969        +75     
- Partials       4579       4606        +27

Ehco1996

restLGTM

Ehco1996 · 2022-04-18T01:08:48Z

dm/checker/checker.go

+		if len(c.stCfgs) > 0 && len(c.instances) > 0 {
+			switch c.stCfgs[0].Mode {
+			case config.ModeAll:
+				c.checkList = append(c.checkList, checker.NewLoaderConnAmountCheker(c.instances[0].targetDB, c.stCfgs))


as this check is somthing about connection count, do we need to put this item in the head of the check queue?

Will fix it.

Checker list is parallel. So it doesn't matter.

tiflow/dm/pkg/checker/real_checker.go

Lines 131 to 139 in 536e182

for i, checker := range checkers {

wg.Add(1)

go func(i int, checker RealChecker) {

defer wg.Done()

result := checker.Check(ctx)

result.ID = uint64(i)

resultCh <- result

}(i, checker)

}

okJiang · 2022-04-18T03:01:05Z

dm/checker/checker.go

+		if len(c.stCfgs) > 0 && len(c.instances) > 0 {
+			switch c.stCfgs[0].Mode {
+			case config.ModeAll:
+				c.checkList = append(c.checkList, checker.NewLoaderConnAmountCheker(c.instances[0].targetDB, c.stCfgs))


Checker list is parallel. So it doesn't matter.

tiflow/dm/pkg/checker/real_checker.go

Lines 131 to 139 in 536e182

for i, checker := range checkers {

wg.Add(1)

go func(i int, checker RealChecker) {

defer wg.Done()

result := checker.Check(ctx)

result.ID = uint64(i)

resultCh <- result

}(i, checker)

}

okJiang · 2022-04-18T03:03:02Z

dm/pkg/checker/conn_checker.go

+		return result
+	}
+	var rows *sql.Rows
+	rows, err = baseConn.QuerySQL(tcontext.NewContext(ctx, log.L()), "SHOW VARIABLES LIKE '%max_connections%'")


You can reuse func GetMaxConnections.

Yes. But GetMaxConnections has no retry strategy. Users have to retry by themselves if connection errors occur (#5015).

Actually, there are a bunch of different pkgs used for querying SQL, varying from implementation and retry strategy. BaseConn is good to have decent retry strategy as well as encapsulation. Should we refactor and integrate them as a whole package?

Currently, @lance6716 is planning to refactor the syncer. Idk if this could be part of the plan cuz it seems like a big one to do.

Ehco1996

LGTM

lichunzhu

Do we need a checker for dumpling?

lichunzhu · 2022-04-18T06:37:54Z

dm/pkg/checker/conn_checker.go

+func NewLoaderConnAmountCheker(targetDB *conn.BaseDB, stCfgs []*config.SubTaskConfig) RealChecker {
+	return &LoaderConnAmountChecker{
+		connAmountChecker: newConnAmountChecker(targetDB, stCfgs, func(stCfgs []*config.SubTaskConfig) int {
+			loaderConn := 0


loader's checkpoint db will use some connections. Does this part need to be calculated?

I will have a look at it.

Every unit maintains 1 db connection for checkpoint. I added it and left a comment. PTAL again😀

dm/pkg/checker/conn_checker.go

buchuitoudegou · 2022-04-18T10:16:17Z

/run-verify

buchuitoudegou · 2022-04-18T11:12:34Z

/run-dm-integration-test

buchuitoudegou · 2022-04-19T02:24:24Z

/run-dm-integration-test

dm/dm/config/checking_item.go

lance6716 · 2022-04-19T03:05:39Z

dm/pkg/checker/conn_checker.go

+			syncerConn := 0
+			for _, stCfg := range stCfgs {
+				// syncer's worker and checkpoint (always keeps one db connection)
+				syncerConn += stCfg.SyncerConfig.WorkerCount + 1


maybe syncer has a DDL connection?

lance6716 · 2022-04-19T03:06:44Z

dm/tests/dmctl_command/run.sh

@@ -142,6 +145,39 @@ function run() {
 	fi
 }

+function test_full_mode_conn() {


please check the actual connection is same as you expected by SHOW PROCESSLIST

Did you mean I should start-task and see if they are as expected?

Yes. if you do this check, you can find the forgotten checkpoint / DDL / dump control connections on your own.

lance6716

wrong action

lance6716 · 2022-07-29T03:12:42Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8315/pipeline

sequence_sharding_optimistic failed

/run-dm-integration-test

please open an issue for them. I'm afraid we will forget them.

buchuitoudegou · 2022-07-29T03:26:42Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8315/pipeline
sequence_sharding_optimistic failed
/run-dm-integration-test

please open an issue for them. I'm afraid we will forget them.

#6515

buchuitoudegou · 2022-07-29T04:09:14Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8320/pipeline

shardddl3-1

/run-dm-integration-test

buchuitoudegou · 2022-07-29T04:14:07Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8320/pipeline

shardddl3-1

/run-dm-integration-test

similar to this: #3461

buchuitoudegou · 2022-07-29T04:56:12Z

/run-dm-integration-test

buchuitoudegou · 2022-07-29T05:33:55Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8324/pipeline

/run-dm-integration-test

buchuitoudegou · 2022-07-29T06:14:17Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8326/pipeline

shardddl3-1 again

/run-dm-integration-test

buchuitoudegou · 2022-07-29T06:16:35Z

/run-dm-integration-test

lance6716 · 2022-07-29T06:55:06Z

/run-leak-test

buchuitoudegou · 2022-07-29T07:11:41Z

https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/8329/pipeline

/run-dm-integration-test

buchuitoudegou · 2022-07-29T07:12:25Z

checkpoint_transaction

[2022-07-29T07:07:41.645Z] [2022/07/29 15:04:00.319 +08:00] [ERROR] [baseconn.go:184] ["execute statement failed"] [task=sequence_sharding_removemeta] [unit=load] [query="CREATE TABLE `t_target` (`id` bigint NOT NULL AUTO_INCREMENT,`uid` int DEFAULT NULL,`name` varchar(20) COLLATE utf8mb4_bin DEFAULT NULL,`c` int DEFAULT NULL,`d` int DEFAULT NULL,`e` int DEFAULT NULL,PRIMARY KEY (`id`),UNIQUE KEY `uid` (`uid`),KEY `c` (`c`),KEY `d` (`d`),KEY `e` (`e`)) ENGINE=InnoDB AUTO_INCREMENT=5 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;"] [argument="[]"] [error="Error 1050: Table 't_target' already exists"]

lance6716 · 2022-07-29T07:15:17Z

checkpoint_transaction

[2022-07-29T07:07:41.645Z] [2022/07/29 15:04:00.319 +08:00] [ERROR] [baseconn.go:184] ["execute statement failed"] [task=sequence_sharding_removemeta] [unit=load] [query="CREATE TABLE `t_target` (`id` bigint NOT NULL AUTO_INCREMENT,`uid` int DEFAULT NULL,`name` varchar(20) COLLATE utf8mb4_bin DEFAULT NULL,`c` int DEFAULT NULL,`d` int DEFAULT NULL,`e` int DEFAULT NULL,PRIMARY KEY (`id`),UNIQUE KEY `uid` (`uid`),KEY `c` (`c`),KEY `d` (`d`),KEY `e` (`e`)) ENGINE=InnoDB AUTO_INCREMENT=5 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;"] [argument="[]"] [error="Error 1050: Table 't_target' already exists"]

this is an acceptable error, not the root cause. please open an issue

ti-chi-bot · 2022-07-29T07:25:21Z

@buchuitoudegou: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

buchuitoudegou · 2022-07-29T07:58:58Z

/run-verify

buchuitoudegou · 2022-07-29T08:22:09Z

/run-verify

ti-chi-bot · 2022-07-29T08:41:46Z

In response to a cherrypick label: cannot checkout release-2.0.6: error checking out release-2.0.6: exit status 1. output: error: pathspec 'release-2.0.6' did not match any file(s) known to git

ti-chi-bot · 2022-07-29T08:42:19Z

In response to a cherrypick label: cannot checkout release-2.0.7: error checking out release-2.0.7: exit status 1. output: error: pathspec 'release-2.0.7' did not match any file(s) known to git

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-07-29T08:42:55Z

In response to a cherrypick label: new pull request created: #6535.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-07-29T08:43:33Z

In response to a cherrypick label: new pull request created: #6536.

ti-chi-bot · 2022-07-29T08:44:10Z

In response to a cherrypick label: new pull request created: #6537.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-07-29T08:44:45Z

In response to a cherrypick label: cannot checkout release-2.0.5: error checking out release-2.0.5: exit status 1. output: error: pathspec 'release-2.0.5' did not match any file(s) known to git

fix: too many connection error

1e6a05f

buchuitoudegou added the area/dm Issues or PRs related to DM. label Apr 15, 2022

Ehco1996 reviewed Apr 18, 2022

View reviewed changes

okJiang reviewed Apr 18, 2022

View reviewed changes

add ut for checker

415bb5f

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 18, 2022

Ehco1996 approved these changes Apr 18, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 18, 2022

lichunzhu reviewed Apr 18, 2022

View reviewed changes

sunzhaoyang mentioned this pull request Apr 18, 2022

Add description of ignore-checking-items pingcap/docs-cn#8790

Merged

16 tasks

feat: add dumper checker

b42a9a4

lance6716 approved these changes Apr 19, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 19, 2022

lance6716 requested changes Apr 19, 2022

View reviewed changes

Merge branch 'master' into many-conn-err

c235fb0

Merge branch 'master' into many-conn-err

c48b5cb

ti-chi-bot merged commit 566357e into pingcap:master Jul 29, 2022

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Jul 29, 2022

This is an automated cherry-pick of pingcap#5185

ac9d932

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jul 29, 2022

checker(dm): check the number of connections before starting (#5185) #6535

Closed

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Jul 29, 2022

This is an automated cherry-pick of pingcap#5185

0e128f5

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jul 29, 2022

checker(dm): check the number of connections before starting (#5185) #6536

Closed

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Jul 29, 2022

This is an automated cherry-pick of pingcap#5185

cd39bbd

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jul 29, 2022

checker(dm): check the number of connections before starting (#5185) #6537

Closed

	for i, checker := range checkers {
	wg.Add(1)
	go func(i int, checker RealChecker) {
	defer wg.Done()
	result := checker.Check(ctx)
	result.ID = uint64(i)
	resultCh <- result
	}(i, checker)
	}

checker(dm): check the number of connections before starting #5185

checker(dm): check the number of connections before starting #5185

Conversation

buchuitoudegou commented Apr 15, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 15, 2022 • edited Loading

buchuitoudegou commented Apr 15, 2022

codecov-commenter commented Apr 15, 2022 • edited Loading

Codecov Report

Ehco1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 left a comment

Choose a reason for hiding this comment

lichunzhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchuitoudegou commented Apr 18, 2022

buchuitoudegou commented Apr 18, 2022

buchuitoudegou commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

lance6716 commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

lance6716 commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

buchuitoudegou commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022

buchuitoudegou commented Apr 15, 2022 •

edited

Loading

ti-chi-bot commented Apr 15, 2022 •

edited

Loading

codecov-commenter commented Apr 15, 2022 •

edited

Loading

lance6716 Apr 19, 2022 •

edited

Loading